🔴 Automated review · pr-review-watcher · 1029b8c

Summary

Pins the production release signing public key into install.sh/install.ps1 and turns signature verification on by default for the release channel, with env/param escape hatches, current+next multi-key rotation, and CI gates (verify_pinned_keys.py, release_readiness.py) meant to keep the pinned key and the CI signing key in lockstep. Verdict: Needs work — the installer/Rust trust logic is correct and fails closed, but the CI consistency gate that couples the pinned constant to the CI signing key has a soundness bug that defeats its purpose during key rotation. Verified: RSA-2048 key is valid and byte-identical across all three embeddings + docs/keys/; both self-tests and release_readiness.py --self-test pass; installer multi-key verify loops fail closed (empty/mismatched key → fail, no silent bypass), escape-hatch precedence correct in shell/PS/Rust; the two blocking findings below were reproduced with runnable PoCs. Blocking: 2 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

scripts/verify_pinned_keys.py:146 (check_pinned_keys) — whole-file containment is an unsound equality proxy. The check does canonical_body in base64_only(source_file) over the entire source file, not against the specific pinned constant. So a wrong/tampered PINNED_RELEASE_PUBLIC_KEY_CURRENT passes as long as the canonical body appears anywhere in the file. Reproduced: with CURRENT set to a wrong key and the real canonical body present in the NEXT slot (a plausible mid-rotation state — exactly the multi-key flow this PR adds) or in a stale comment, check_pinned_keys() returns success. This is the only gate that ties the installer-pinned constant to the CI signing key (release_readiness.py verifies archives against the CI key, not the pinned constant), so its docstring claim of being a "reliable equality proxy" / "single source of truth" is the load-bearing property — and it's violated. Fix: extract each named pinned constant's own PEM body from its source (parse the specific PINNED_..._CURRENT/_NEXT assignment / Rust const) and compare that body for equality, instead of substring-scanning the whole file.

.github/workflows/ci.yml:68-79 — the PR-time consistency check is skippable for docs/keys/**-only changes. The new verify_pinned_keys.py step is gated on needs.changes.outputs.heavy == 'true', but the heavy paths-filter does not include docs/keys/**. A PR that edits only docs/keys/rocm-cli-release-current-public.pem (rotating the canonical key without touching an installer) never sets heavy=true, so the consistency check is skipped at review time for precisely the divergence it exists to catch. It compounds with the finding above. (nightly.yml/release.yml run the check unconditionally, so this is a PR-gate hole, not a release bypass — but it should still be closed.) Fix: add docs/keys/** to the heavy filter (and/or run verify_pinned_keys.py unconditionally in ci.yml).

Non-blocking

• .github/workflows/nightly.yml / release.yml (env-block comments) and the PR body reference plans/release-production-trust-rollout.md, which does not exist in the repo — dead reference.
• scripts/release_readiness.py:488,1020 — resolve_release_public_key() uses NamedTemporaryFile(delete=False) and never unlinks on the real main() path (only the self-test cleans up); minor temp-file leak on CI runners.
• install.ps1:183 (Confirm-ArchiveSignature) — $key/$SignaturePath/$ArchivePath passed unquoted to native openssl; a path containing a space breaks the call (fails closed, not a bypass). Pre-existing pattern the PR extended to the new loop var; quote all three.
• actions/checkout@v6 / actions-rust-lang/setup-rust-toolchain@v1 use moving tags (AGENTS.md §6 wants full-SHA pins). Confirmed pre-existing — this PR adds no uses: lines — so out of PR scope, noted for future hardening.